Remove secret from cicd #208
Conversation
There was a problem hiding this comment.
Looking good, and good idea on the testing technique! I left some comments to dig deeper to deploy script to ensure that it is using the new auth method. You may want to adjust the branch trigger for cicd so that it runs on your PR branch pushes so that you can test out the full execution before merging. There shouldn't be an issue with doing multiple (even failed) deployments against our test infrastructure (is that still right @ghidalgo3 )
There was a problem hiding this comment.
LGTM!
I think the required use of use_oidc in the tf may make it difficult for this to be run locally, though that isn't the typical workflow. I think we focus on this secret removal now and shift the burden of local deploys when we cross that bridge.
@elayrocks can you delete the pc-apis secret (make sure to leave the pctasks one for now) before merging this so that the merged run definitely doesn't use the secret? After it's merged, can you delete the old GitHub secrets.AZURE_CREDENTIALS as well?
Description
The motivation of this PR is to remove the client secret in the GitHub actions workflow file
cicd.ymlto improve security.To do this, I reference this page and created federated credentials in service principal which allows authentication without the need for explicit client secret. I set up a new repo secret called
SECURE_AZURE_CREDENTIALS, which does not contain client secret. Also made corresponding changes to authentication in the workflow.Type of change
How Has This Been Tested?
I created federated credential for pull-request, so pull-request can use the service principal as well. It shall be deleted after testing.I tested it by changingpr.yml(link) and make sureaz loginandaz acr loginvia service principal are successful. But deployment has not been tested. I'm afraid it has to be tested after this PR is merged to main and triggers the CI/CD pipeline.I created federated credential for this branch
remove-secret-from-cicd, so it can use the service principal to authenticate to Azure. It will be deleted after testing. Incicd.yml, changed branch trigger toremove-secret-from-cicdand comment out the if clause, so that
build_and_publishanddeployjobs can all be tested once changes are pushed toremove-secret-from-cicd. Check out the result of latest pipeline run and workflow fileChecklist: